More cleanup of load.py #8
Conversation
| @@ -1,7 +1,6 @@ | |||
| PARSONS_FOLDER_NAME = 'parsons_probs' | |||
| PARSONS_FOLDER_PATH = './parsons_probs' | |||
| PARSONS_GLOB = 'parsons_probs/*.py' | |||
| PROBLEM_PATHS = [PARSONS_FOLDER_PATH] | |||
There was a problem hiding this comment.
Given this is always a list with one constant, the added complexity of its existence doesn't seem warranted.
| for path in PROBLEM_PATHS: | ||
| config_files.append(os.path.join(os.path.abspath(path), problem_name.lower() + ".yaml")) | ||
| return load_config_file(config_files) | ||
| path = os.path.join(os.path.abspath(PARSONS_FOLDER_PATH), problem_name.lower() + ".yaml") |
There was a problem hiding this comment.
With the recognition that there is only one path, this logic is shorter, so I combined it with the function above.
| @@ -54,7 +39,9 @@ def problem_name_from_file(filename): | |||
| name = func_sig[:func_sig.index('(')] | |||
| return name | |||
|
|
|||
|
|
|||
| def problem_name_to_file(problem_name, extension): | |||
| return f'{PARSONS_FOLDER_PATH}/{problem_name.lower()}.{extension}' | |||
There was a problem hiding this comment.
It's not clear to me why we use os.path.join above but just an f-string here. Is there a reason? Otherwise I'd like to standardize on one way of creating paths. I imagine os.path.join might take care of weird slash issues or some such.
| @@ -133,7 +133,7 @@ def submit(): | |||
| problem_name = request.form['problem_name'] | |||
| submitted_code = request.form['submitted_code'] | |||
| parsons_repr_code = request.form['parsons_repr_code'] | |||
| fname = f'{PARSONS_FOLDER_PATH}/{problem_name.lower()}.py' | |||
| fname = problem_name_to_file(problem_name, 'py') | |||
There was a problem hiding this comment.
I moved logic into the load file for testability (since this file is so hard to test right now, given the complexity of the okclient import).
| def test_load_config_lowercased(self): | ||
| with self.assertRaises(Exception) as context: | ||
| load.load_config('remove_indexes') | ||
| self.assertIn('remove_indexes.yaml', str(context.exception)) | ||
| load.load_config("remove_indexes") |
There was a problem hiding this comment.
I ran the black formatter on this file so there are some whitespace/quote changes.
| self.assertIn('smartfridge.yaml', str(context.exception)) | ||
| self.assertIn("smartfridge.yaml", str(context.exception)) | ||
|
|
||
| def test_problem_name_to_file(self): |
There was a problem hiding this comment.
The actual change is the addition of this test function.
No description provided.